Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various table fixes #1833

Merged
merged 1 commit into from
May 27, 2024
Merged

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented May 27, 2024

This PR makes a few changes to the _tables.scss file:

  • Matches our table alignment classes with an update to MyST from 2021 (version 0.16.0)
  • Removes the redundant and useless display: table and overflow: auto rules
  • Updates a comment

Fixes #1804.

Copy link
Collaborator Author

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self review

Comment on lines +33 to +41
&.text-left {
text-align: left;
}

&.text-align\:right {
&.text-right {
text-align: right;
}

&.text-align\:center {
&.text-center {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These classes were changed in executablebooks/MyST-Parser#450

Comment on lines -9 to -11
display: table;
overflow: auto;

Copy link
Collaborator Author

@gabalafou gabalafou May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of flip-flop history here:

As I discuss in issue #1824, it's probably a bad idea to change the display property of a table element since that might strip it of its semantics. And there's no need to explicitly put display: table on the table element, because that's what browsers assume.

As for the overflow rule, it only made sense when the table had display: block, as I discuss in PR #1827.

So to reduce head-scratching for future devs, I'm removing both rules.

@dbitouze
Copy link
Contributor

dbitouze commented May 27, 2024

This PR makes a few changes to the _tables.scss file:

Why a so old version of MyST?

Fixes #1804.

Thanks!

@Carreau
Copy link
Collaborator

Carreau commented May 27, 2024

Why a so old version of MyST?

I think what was meant are "this changes has been introduced in myst 0.16 in 2021, so are not recent, therefore safe", not "we want to align to an old version of myst".

@gabalafou
Copy link
Collaborator Author

gabalafou commented May 27, 2024

I think what was meant are "this changes has been introduced in myst 0.16 in 2021, so are not recent, therefore safe", not "we want to align to an old version of myst".

Correct. That's my justification for not leaving a backwards-compatibility layer.

@Carreau Carreau merged commit aabe57d into pydata:main May 27, 2024
30 checks passed
@gabalafou gabalafou deleted the left-right-start-end-table branch May 27, 2024 17:01
ivanov pushed a commit to ivanov/pydata-sphinx-theme that referenced this pull request Jun 5, 2024
This PR makes a few changes to the _tables.scss file:

- Matches our table alignment classes with an [update to MyST from 2021](executablebooks/MyST-Parser#450) (version [0.16.0](https://github.com/executablebooks/MyST-Parser/releases/tag/v0.16.0))
- Removes the redundant and useless `display: table` and `overflow: auto` rules
- Updates a comment

Fixes pydata#1804.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table cell right alignment is wrong
3 participants